Closed Bug 1215610 Opened 10 years ago Closed 10 years ago

Conditional jump or move depends on uninitialised value(s) in GenerateSetSlot

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID
Tracking Status
firefox44 --- affected

People

(Reporter: bc, Unassigned)

Details

(Keywords: sec-high, valgrind)

Attachments

(2 files)

I did a partial run of jstestbrowser under valgrind for a debug build on Fedora 22. I stopped it after a few hours but found these messages fairly early. If you find this helpful, I'll complete the run. http://hg.mozilla.org/mozilla-central/rev/e193b4da0a8c1025aa76a403c64663ff1cd41709 vncserver :1 vncviewer :1 (DISPLAY=:1.0 make -C firefox-debug jstestbrowser EXTRA_TEST_ARGS=' --timeout=86400 --debugger=/usr/local/bin/valgrind --debugger-args=" --track-origins=yes --fair-sched=yes --smc-check=all-non-file --soname-synonyms=somalloc=NONE --vex-iropt-register-updates=allregs-at-mem-access --partial-loads-ok=yes --error-limit=no --trace-children=yes --child-silent-after-fork=yes --trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep,*/bin/java --tool=memcheck --stats=yes --read-inline-info=yes --fullpath-after=/mozilla/builds/nightly/mozilla"' ) 2>&1 | tee jsreftest-valgrind.2015-10-16.txt I see several of these messages. (See attachment) ==9316== Conditional jump or move depends on uninitialised value(s) ==9316== at 0xAAD77B6: GenerateSetSlot (/js/src/jit/IonCaches.cpp:2276) ... ==9316== Uninitialised value was created by a stack allocation ==9316== at 0xAAD9D97: js::jit::SetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, unsigned long, JS::Handle<JSObject*>, JS::Handle<JS::Value>) (/js/src/jit/IonCaches.cpp:3415) // Guard that the incoming value is in the type set for the property // if a type barrier is required. if (needsTypeBarrier) { // We can't do anything that would change the HeapTypeSet, so // just guard that it's already there. ==> if (checkTypeset) { masm.push(object); CheckTypeSetForWrite(masm, obj, shape->propid(), object, value, &failurePopObject); masm.pop(object); } } bool SetPropertyIC::update(JSContext* cx, HandleScript outerScript, size_t cacheIndex, HandleObject obj, HandleValue value) ==>{ The following does not initialize checkTypeset bool checkTypeset; canCache = CanAttachNativeSetProp(cx, obj, id, cache.value(), cache.needsTypeBarrier(), &holder, &shape, &checkTypeset); if (!addedSetterStub && canCache == CanAttachSetSlot) { RootedNativeObject nobj(cx, &obj->as<NativeObject>()); if (!cache.attachSetSlot(cx, outerScript, ion, nobj, shape, checkTypeset)) return false; addedSetterStub = true; }
Keywords: sec-high
Group: core-security → javascript-core-security
Hm I'm pretty sure this is a false positive. GCC is likely optimizing the code to calculate (needsTypeBarrier & checkTypeset) and branch on the result of that. checkTypeset is set whenever needsTypeBarrier is true so this optimization is fine (if needsTypeBarrier is false, we won't hit this branch no matter what value checkTypeset has), but Valgrind doesn't know that. NI myself to double check. We can probably refactor the code a bit to be clearer and avoid this issue.
Flags: needinfo?(jdemooij)
I cannot reproduce this even with a full run (all 6995 tests). This is with a build by gcc-4.9.2, optimisation flags "-Og -g". What flags are you optimising with?
-g -O
> checkTypeset is set whenever needsTypeBarrier is true Are you sure about that? Valgrind is saying that checkTypeset is not initialized when needsTypeBarrier is true.
(In reply to Nicholas Nethercote [:njn] from comment #4) This might be one of those "a && b" --> "b && a iff a is provably false when b is undefined" transformations. Mostly they seem to be a problem only at -O2 though, so I am surprised Bob can reproduce it a -O. Until such time as someone can reproduce this, it's all just speculation. I'll try with -g -O.
I can reproduce this with -g -O. I suspect it's a false positive, but some peering at the machine code didn't 100% convince me. Is there a way to re-run just this test (jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.5.2-1.js) instead of the whole suite? That takes about 2 hours. I tried various combinations of TEST_PATH= and just adding the path, but to no avail.
Flags: needinfo?(bob)
Unfortunately no. The test selection appears to be completely broken and the ability to run the tests individually in the browser is prevented by the use of SpecialPowers which will force a crash if run outside of the automation scripts.
Flags: needinfo?(bob)
(In reply to Bob Clary [:bc:] from comment #7) > Unfortunately no. The test selection appears to be completely broken and the > ability to run the tests individually in the browser is prevented by the use > of SpecialPowers which will force a crash if run outside of the automation > scripts. You should be able to set security.turn_off_all_security_so_that_viruses_can_take_over_this_computer to true (seriously) and get SpecialPowers to work, though please do not browser the web using that for the reason that is described in the name of the preference. ;)
mccr8: Great thanks. I ran a test ecma/Array/15.4.5.1-2.js with the original old build 20151015053239 via valgrind --track-origins=yes --fair-sched=yes --smc-check=all-non-file --soname-synonyms=somalloc=NONE --vex-iropt-register-updates=allregs-at-mem-access --partial-loads-ok=yes --error-limit=no --trace-children=yes --child-silent-after-fork=yes --trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep,*/bin/java --tool=memcheck --stats=yes --read-inline-info=yes --fullpath-after=/mozilla/builds/nightly/mozilla ./firefox -P jsreftest 'file:///mozilla/builds/nightly/mozilla/js/src/tests/jsreftest.html?test=ecma/Array/15.4.5.1-2.js;language=type;text/javascript' > /tmp/bug-1215610-valgrind.txt 2>&1 with a special profile initialized with the same preferences as js/src/tests/user.js though you could probably get by with just user_pref("security.turn_off_all_security_so_that_viruses_can_take_over_this_computer", true); user_pref("dom.max_script_run_time", 0); user_pref("dom.max_chrome_script_run_time", 0); I'll attach the log, but on first glance this did not reproduce the original error message but did find some others I had already seen plus perhaps a new one or two. I forgot the --show-mismatched-frees=no so it also shows the mismatched free/delete message which can be ignored. The full test runs using the reftest harness re-use the window and running different tests so that may contain the root issue. I didn't run the other tests where the original valgrind message appeared though, so perhaps one of them would reproduce it.
log for ecma/Array/15.4.5.1-2.js Note I packaged the specialpowers.xpi from the dist/xpi-stage directory and installed it into the special profile with the appropriate preferences set.
Flags: needinfo?(mconley)
(In reply to Bob Clary [:bc:] from comment #11) > I wonder if mconley's experience with > http://mikeconley.ca/blog/2015/10/30/a-printing-story-and-a-psa-outparams- > over-the-ipc-layer-might-not-behave-like-youd-expect/ might be relevant here. I appreciate the vote of confidence given my recent experience in that blog post, but from reading over this bug, I'm afraid it's a bit over my head (I've never worked with the JIT stuff). I don't think I'm going to be much help. :/
Flags: needinfo?(mconley)
Given that I can reproduce this at -g -O but not at -g -Og (which is less aggressive in optimisation), I believe this is likely a false positive and that we should close the bug.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Flags: needinfo?(jdemooij)
Group: javascript-core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: